-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Updated RegexFilter implementtation with changes from 2.x branch merge to main [3.x] (#3086) #3513
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
… createFilter. (apache#3086) 8d05a7
+ made AbstractFiltter.AbstractFilterBuilder onMatch/onMismatch fields protected + added AbstractFilter(AbstractFilterBuilder) constructor + added RegexFilter.Builder implementation + added RegexFilter(Builder) constructor + moved RegexFilter Pattern compile into constructor + added fields to persist configuration propertties + getters (regexExpression, patternFlags) + changed private constructor to accept builder as argument + renamed private method 'targetMessageTest' to more approprriate 'getMessageTextByType' + added Javadoc + grouped deprecations
+ added validation checks to RegexFilter + added JVerify nullability annotations to RegexFilter + updated javadoc + replaced deprecated usages of CompositeFilter#getFilters with CompositeFilter#getFiltersArray in AbstractFilterableTest
…ot yet been merged to main (apache#3086)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for reviewing it so late. The PR looks very nice, I would just address the remarks below:
@PluginBuilderAttribute(ATTR_ON_MATCH) | ||
private Result onMatch = Result.NEUTRAL; | ||
protected Result onMatch = Result.NEUTRAL; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is already a getter and a setter. Making this protected
is not needed.
@PluginBuilderAttribute(ATTR_ON_MISMATCH) | ||
private Result onMismatch = Result.DENY; | ||
protected Result onMismatch = Result.DENY; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is already a getter and a setter. Making this protected
is not needed.
/** | ||
* Returns the compiled regular-expression pattern. | ||
* @return the pattern (will never be {@code null} | ||
*/ | ||
public Pattern getPattern() { | ||
return this.pattern; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since there is already a getRegex
method below, I think this method can be removed.
@PluginFactory | ||
public static RegexFilter createFilter( | ||
// @formatter:off | ||
@PluginAttribute final String regex, | ||
@PluginElement final String[] patternFlags, | ||
@PluginAttribute final Boolean useRawMsg, | ||
@PluginAttribute final Result onMatch, | ||
@PluginAttribute final Result onMismatch) | ||
// @formatter:on | ||
throws IllegalArgumentException, IllegalAccessException { | ||
if (regex == null) { | ||
LOGGER.error("A regular expression must be provided for RegexFilter"); | ||
return null; | ||
} | ||
return new RegexFilter(useRawMsg, Pattern.compile(regex, toPatternFlags(patternFlags)), onMatch, onMismatch); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For backward compatibility, we can not remove this method, but we can:
- Remove the
@PluginFactory
annotation. - Deprecated it.
- Rewrite it in terms of builder calls.
public static Builder newBuilder() { | ||
return new Builder(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A plugin builder factory must be annotated with @PluginBuilderFactory
instead of @PluginFactory
.
Fixes #3086
RegexFilter(Builder)
private constructorMessage#getFormat()
#2773 from 2.x branch that were only partially merged to mainAbstractFilter(Builder)
constructorNOTE: cherry-picking changes directly from 2.x branch not possible due to missing APIs, changed 3.x Plugin design/imports, etc.